-
Notifications
You must be signed in to change notification settings - Fork 10
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Theodore/proofnarrowing #34
Conversation
Going above 20 was quickly becoming too slow anyway, so no real point in adding an extra dependency and more indirection.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Based on the new test (which is very nice, btw!) this seems to work when the inputs are well-constructed. Unfortunately, it's pretty difficult to be confident about the correctness at a logical level given the current state of the code. Since this is pretty involved logic, I think it's probably worth investing in even more detailed explanations (probably including diagrams) in the core functions.
Actually your comment about the ranges is spot-on - the current test was missing a whole bunch of edge cases due to being non-inclusive. Whoops! Happy to add some more comments and explanations, especially now that I've slept on it and can look at it from a slight distance. |
54b4e25
to
f85d6b3
Compare
By the way, the codecov.io token seems dead, so the badge in the README isn't updating anymore |
- reverted the tendermint version bump - fixed edge cases on trees of total size 0 and 1 - fixed incorrect implementation in nmt_proof.rs (now just wraps the simple proof.rs) - fixed missing test cases for the above two issues - cleaned up inconsistent parameter naming to be more clear - cleaned up large argument sets in a few places to make data flow easier to follow - documented all assertions with a description of why they should never break - added documentation to the functions and descriptions of the narrowing logic
f85d6b3
to
b4c9fb9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for making a pass on the naming and docs. It's much easier to understand this time around. Looks great, thanks @theodorebugnet!
Add proof narrowing capabilities to the tree: given a range proof, supply two sets of leafs (contiguous with the edges of the original range), to generate a proof for the remaining sub-range.
This feature is useful for generating proofs of individual blobs in a Celestia namespace using the full-namespace proof returned by the
GetSharesByNamespace
RPC call.